Skip to content

Fill 17.x/ReleaseNotes with my works#789

Merged
tru merged 2 commits intollvm:release/17.xfrom
chapuni:relnotes/17
Nov 28, 2023
Merged

Fill 17.x/ReleaseNotes with my works#789
tru merged 2 commits intollvm:release/17.xfrom
chapuni:relnotes/17

Conversation

@chapuni
Copy link
Contributor

@chapuni chapuni commented Nov 27, 2023

Feel free to correct, rephrase, and revise this. Thank you,.

Copy link

@ornata ornata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments to make English prose more idiomatic.

``MVT``.

* ``Intrinsics.td`` is able to emit definitions of
``TypeSig``. ``IntrinsicEmitter`` doesn't depend on
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

IntrinsicEmitter no longer depends on MachineValueTypes.h

The sentence you wrote is correct. But the suggestion is more idiomatic.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, I think this should be split into 3 bullet points:

  • Intrinsics.td now emits TypeSig definitions
  • IntrinsicEmitter no longer depends on TypeSig
  • IntrinsicEmitter no longer depends on MachineValueTypes.h

``TypeSig``. ``IntrinsicEmitter`` doesn't depend on
``MachineValueTypes.h`` anymore.

* ``llvm/CodeGen/MachineValueType.h`` is moved from ``llvm/Support``.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is moved from

Do you mean removed from? Were the types in llvm/CodeGen/MachineValueType.h moved to ValueTypes.td?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see now. I think this would be clearer:

  • MachineValueType.h is moved from llvm/Support to llvm/CodeGen.


* ``llvm-min-tblgen`` is internally introduced to build LLVM public
headers. Note that ``llvm-tblgen`` depends on `GenVT.inc` that is
generated by ``llvm-min-tblgen``. Specify the external
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

Builds using LLVM_TABLEGEN to specify a pre-built tablegen executable should use llvm-tblgen.

I think you can drop the part about llvm-tblgen being a superset of llvm-min-tblgen. It is implied above.

Changes to building LLVM
------------------------

* ``llvm-min-tblgen`` is internally introduced to build LLVM public
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"introduced internally" seems more natural than "internally introduced"

``name=value``.

* Each tablegen backend can declare itself as self-contained with
``llvm::TableGen::Emitter`` as the registry. You don't need to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer "users" to "you" in documentation.


* Each tablegen backend can declare itself as self-contained with
``llvm::TableGen::Emitter`` as the registry. You don't need to
append entries and options to ``TableGen.cpp``. For now, It is
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/It is/it is/

@chapuni
Copy link
Contributor Author

chapuni commented Nov 28, 2023

@ornata Thanks for the review. I have followed your suggestions.

I am ready, @tru could you cherry-pick this please?

@tru tru merged commit b3a30dc into llvm:release/17.x Nov 28, 2023
@chapuni chapuni deleted the relnotes/17 branch November 28, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants